New Rule S6966: Awaitable method should be used by martin-strecker-sonarsource · Pull Request #9101 · SonarSource/sonar-dotnet

martin-strecker-sonarsource · 2024-04-15T15:47:42Z

Fixes #9096

analyzers/its/expected/BlazorSample/S6966-BlazorSample-net7.0.json

...ers/its/expected/CSharpLatest/S6966-NetCore31.MVC.ConfigurableRules-netcoreapp3.1.Views.json

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6966-AspNetCore6-net6.0.json

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6966-AspNetCore7-net7.0.json

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6966-AspNetCore8-net8.0.json

analyzers/its/expected/RazorSample/S6966-RazorSample-net7.0.json

analyzers/its/expected/akka.net/S6966-Akka.Cluster.Sharding-netstandard2.0.json

martin-strecker-sonarsource · 2024-04-17T12:34:35Z

analyzers/src/SonarAnalyzer.Common/Extensions/ISymbolExtensions.Roslyn.cs

+
+namespace Microsoft.CodeAnalysis.Shared.Extensions;
+
+[GeneratedCode("Copied from Roslyn", "ca66296efa86bd8078508fe7b38b91b415364f78")]


Note to myself:
Needs to be changed to [ExcludeFromCodeCoverage]
https://sonarsource.slack.com/archives/C06S7H06Z4Y/p1713354144963039?thread_ts=1713264344.421699&cid=C06S7H06Z4Y

martin-strecker-sonarsource · 2024-04-17T15:58:16Z

analyzers/tests/SonarAnalyzer.Test/Rules/UseAwaitableMethodTest.cs

+#if NET
+
+    [TestMethod]
+    public void UseAwaitableMethod_CS_Test() =>


This snippet is for testing only and will be removed before merging

zsolt-kolbay-sonarsource

LGTM. Left a bunch of polishing comments, but nothing major.

analyzers/src/SonarAnalyzer.CSharp/Rules/UseAwaitableMethod.cs

zsolt-kolbay-sonarsource · 2024-04-17T13:31:35Z

analyzers/tests/SonarAnalyzer.Test/TestCases/UseAwaitableMethod.cs

+
+        ReturnMethod(); // Noncompliant
+        _ = ReturnMethod(); // Noncompliant
+        this.ReturnMethod().ReturnMethod().ReturnMethod();


Add the following test case:

await ReturnMethod().ReturnMethodAsync(); // Noncompliant // ^^^^^^^^^^^^^^

zsolt-kolbay-sonarsource · 2024-04-17T13:45:28Z

analyzers/tests/SonarAnalyzer.Test/TestCases/UseAwaitableMethod.cs

+    public static C operator -(C c) => default(C);
+    public static C operator -(C c1, C c2) => default(C);
+    public static C operator !(C c) => default(C);
+    public static C operator ~(C c) => default(C);


The ~ operator is not used.

analyzers/tests/SonarAnalyzer.Test/TestCases/UseAwaitableMethod.cs

zsolt-kolbay-sonarsource · 2024-04-17T14:57:01Z

analyzers/tests/SonarAnalyzer.Test/TestCases/UseAwaitableMethod_CSharp9.cs

+using System;
+
+    var ms = new MemoryStream();
+    ms.Dispose(); // Noncompliant {{Await DisposeAsync instead.}}


The rule can be extended to also raise a warning a using statement without the await keyword in an async context (or maybe as a new rule?).

using var ms1 = new MemoryStream(); // Noncompliant await using var ms2 = new MemoryStream(); // Compliant

WDYT?

👍 That requires a different implementation. Please create a Rule Idea issue.

analyzers/src/SonarAnalyzer.CSharp/Rules/UseAwaitableMethod.cs

zsolt-kolbay-sonarsource · 2024-04-17T15:57:46Z

analyzers/src/SonarAnalyzer.CSharp/Rules/UseAwaitableMethod.cs

+        return wellKnownExtensionMethodContainer;
+    }
+
+    private static IEnumerable<IMethodSymbol> GetMethodSymbolsInScope(string methodName, WellKnownExtensionMethodContainer wellKnownExtensionMethodContainer,


Move this and the following method below FindAwaitableAlternatives.

analyzers/src/SonarAnalyzer.CSharp/Rules/UseAwaitableMethod.cs

analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs

martin-strecker-sonarsource · 2024-04-18T14:35:37Z

Other well known extension methods to look at:
https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/dotnet/runtime%24+file:Extensions.cs+Async&patternType=regexp&sm=0

zsolt-kolbay-sonarsource · 2024-04-18T16:00:38Z

analyzers/src/SonarAnalyzer.CSharp/Rules/UseAwaitableMethod.cs

+        if (awaitableRoot is { Parent: AwaitExpressionSyntax })
+        {
+            return ImmutableArray<ISymbol>.Empty; // Invocation result is already awaited.
+        }
+        if (invocationExpression.EnclosingScope() is { } scope && !IsAsyncCodeBlock(scope))
+        {
+            return ImmutableArray<ISymbol>.Empty; // Not in an async scope
+        }


The two conditions can be merged into a single if statement.

zsolt-kolbay-sonarsource

LGTM

Co-authored-by: Zsolt Kolbay <121798625+zsolt-kolbay-sonarsource@users.noreply.github.com>

sonarqubecloud · 2024-04-19T12:42:57Z

Quality Gate passed

Issues
0 New issues
5 Accepted issues

Measures
0 Security Hotspots
98.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarqubecloud · 2024-04-19T12:43:18Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

This reverts commit 02fcc9e.

martin-strecker-sonarsource · 2024-04-22T09:21:57Z

Peach validation:

There are AD0001 -> fix is needed
FP/TP rate -> still needs to be done

Performance

The rule is about 3 times as expensive as other rules, like ImplementIDisposableCorrectly or InsteadOfAny. In some projects, it is 15 to 20 times as expensive.
On average it is 2% as expensive as SymbolicExecutionRunner, but can be up to 20-30% as expensive in some projects.
It is 40% as expensive as CopyPasteTokenAnalyzer, but can be up to 5 times as expensive.
The data is shown here.

martin-strecker-sonarsource · 2024-04-22T13:13:15Z

Peach validation:

430 issues found. All are TP with the following exceptions:

FP https://peach.aws-prd.sonarsource.com/project/issues?issues=AY75D7xngjvR54bV5RI9&open=AY75D7xngjvR54bV5RI9&id=aspnetcore-health JsonConvert.SerializeObjectAsync is (probably) marked as [Obsolete].
FP https://peach.aws-prd.sonarsource.com/project/issues?issues=AY747pzkgjvR54bVq_RP&open=AY747pzkgjvR54bVq_RP&id=bitwarden-core do not raise in expression trees.
FP https://peach.aws-prd.sonarsource.com/project/issues?issues=AY75jkdSWL6xA57ilkHD&open=AY75jkdSWL6xA57ilkHD&id=dotnet-runtime undetectable: The sync and async method are either called based on a condition. There are a lot of such patterns in the runtime repo.
TP https://peach.aws-prd.sonarsource.com/project/issues?issues=AY75jjunWL6xA57ilkHB&open=AY75jjunWL6xA57ilkHB&id=dotnet-runtime Await in finally block is allowed since C#6.

Most of the violations originate from (in that order)

Stream
Database / EF
CancelationTokenSource
Custom user methods or nuget package methods
Dispose
XmlWriter

We also have found issues in razor files (@renderSection and the like).

martin-strecker-sonarsource commented Apr 17, 2024

View reviewed changes

martin-strecker-sonarsource requested a review from zsolt-kolbay-sonarsource April 17, 2024 11:54

martin-strecker-sonarsource marked this pull request as ready for review April 17, 2024 11:59

martin-strecker-sonarsource commented Apr 17, 2024

View reviewed changes

zsolt-kolbay-sonarsource approved these changes Apr 18, 2024

View reviewed changes

martin-strecker-sonarsource commented Apr 18, 2024

View reviewed changes

analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs Show resolved Hide resolved

analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs Show resolved Hide resolved

martin-strecker-sonarsource requested a review from zsolt-kolbay-sonarsource April 18, 2024 12:43

zsolt-kolbay-sonarsource reviewed Apr 18, 2024

View reviewed changes

zsolt-kolbay-sonarsource approved these changes Apr 19, 2024

View reviewed changes

martin-strecker-sonarsource added 19 commits April 19, 2024 14:25

Scaffold rule

521b41b

Split test cases and fix compiler errors

3e44c6b

Remove CoreMetadataReference.SystemLinqQueryable

a6ddf4e

Remove SystemComponentModelTypeConverter facade

3ca5936

Add ISymbolExtensions for IsAwaitableNonDynamic

81f0c08

WIP: First implementation

0832792

Make tests green

7c06ec1

Fix test case for Net48

efbd46c

Refactor

3d77f07

Fix test failures

be4cba5

ITs Akka

8fb42ab

Rework type lookups for candidates

6a27444

Add FNs for local functions.

d05eb3c

Refactorings

6f26336

Improve await placement

df0c267

Remove uneccessary Parenthesis removal

3c47551

Add more overload tests

42d8966

Address SQ issues

ef44048

Code smell

a5274db

martin-strecker-sonarsource and others added 14 commits April 19, 2024 14:25

Update ITs

38dc0e2

Apply suggestions from code review

8ab8ee9

Co-authored-by: Zsolt Kolbay <121798625+zsolt-kolbay-sonarsource@users.noreply.github.com>

Clean-up

9acfd85

Add support for SocketTaskExtensions

5b9eb09

FP: Fix support for lambdas

fad1172

Fix EnclosingScope and tests

2ad3efa

Simplify test case

e7872d1

Use AnalyzerLanguage and expose OutputKind in SyntaxNodeExtensionsTest

d234de4

Fix conditional compilation

83fcd4f

Add EnumMember and Parameter to EnclosingScopeSyntaxKinds and tests

84c541e

Fix ISymbolExtensions annotations for Roslyn copied code.

f88f805

Improve IsAsyncCodeBlock and add more tests

3e31d6b

Remove whitespace

6ef7f51

Unify IsAsyncCodeBlock cases

3d79d9d

zsolt-kolbay-sonarsource force-pushed the Martin/S6966 branch from ab1fc63 to 3d79d9d Compare April 19, 2024 12:25

zsolt-kolbay-sonarsource merged commit 02fcc9e into master Apr 19, 2024

zsolt-kolbay-sonarsource deleted the Martin/S6966 branch April 19, 2024 15:59

martin-strecker-sonarsource added a commit that referenced this pull request Apr 22, 2024

Revert "New Rule S6966: Awaitable method should be used (#9101)"

2cf375c

This reverts commit 02fcc9e.

martin-strecker-sonarsource mentioned this pull request Apr 22, 2024

Revert "New Rule S6966: Awaitable method should be used" #9178

Merged

martin-strecker-sonarsource mentioned this pull request Apr 22, 2024

New Rule S6966: Awaitable method should be used #9179

Merged

4 tasks


		namespace Microsoft.CodeAnalysis.Shared.Extensions;

		[GeneratedCode("Copied from Roslyn", "ca66296efa86bd8078508fe7b38b91b415364f78")]

Conversation

martin-strecker-sonarsource commented Apr 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martin-strecker-sonarsource Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

martin-strecker-sonarsource Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

zsolt-kolbay-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zsolt-kolbay-sonarsource Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

zsolt-kolbay-sonarsource Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zsolt-kolbay-sonarsource Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

martin-strecker-sonarsource Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zsolt-kolbay-sonarsource Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martin-strecker-sonarsource commented Apr 18, 2024

Uh oh!

zsolt-kolbay-sonarsource Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

zsolt-kolbay-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 19, 2024

Quality Gate passed

Uh oh!

sonarqubecloud bot commented Apr 19, 2024

Quality Gate passed

Uh oh!

martin-strecker-sonarsource commented Apr 22, 2024

Performance

Uh oh!

martin-strecker-sonarsource commented Apr 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zsolt-kolbay-sonarsource Apr 17, 2024 •

edited

Loading